Outputing subset messages as received (initial work) #233

DemiMarie · 2018-09-14T19:51:23Z

Closes #170.

This is still incomplete; in particular, I need to fix up the test suite.

vkomenda · 2018-09-14T20:37:44Z

src/subset.rs

        if self.decided || self.count_true() < self.netinfo.num_correct() {
            return None;
        }
        // Once all instances of BA have completed, let C ⊂ [1..N] be
        // the indexes of each BA that delivered 1. Wait for the output
        // v_j for each RBC_j such that j∈C. Finally output ∪ j∈C v_j.
        if self.ba_results.len() < self.netinfo.num_nodes() {
-            return None;
+            return Some(SubsetOutput::Contribution(proposer_id.clone(),
+             self.broadcast_results.get(proposer_id).map(|x|x.clone()).expect("internal error: already inserted entry not found")));


rustfmt won't accept this in Travis. Please check the error message.

@vkomenda Fixed

tests/subset.rs

src/subset.rs

DemiMarie · 2018-09-14T22:13:08Z

I made no attempt to avoid unnecessary calls to .clone(). I figured that it was more important to get the functionality working.

src/honey_badger/epoch_state.rs

DemiMarie

Current version: all tests pass

src/honey_badger/epoch_state.rs

src/subset.rs

This outputs subset messages as they are received. All tests pass.

DemiMarie · 2018-09-15T16:43:11Z

Squashed.

afck

Nice work! Thanks for taking on this issue, @DemiMarie!
I highlighted a few points that need to be addressed before we can merge it.

src/honey_badger/epoch_state.rs

afck · 2018-09-16T10:46:31Z

src/honey_badger/epoch_state.rs

+                    let mut map: BTreeMap<N, Vec<u8>> = Default::default();
+                    self.keys.insert(k.clone());
+                    map.insert(k, v);
+                    //step.extend(self.send_decryption_shares(map)?);


Yes, I think we should do that here: it's the optimization that will hopefully speed up Honey Badger.

We should probably also change send_decryption_share[s] to take only one value and get rid of map.

So when I do that, I keep running into:

--- test_dynamic_honey_badger_random_delivery_silent stdout ---- thread 'test_dynamic_honey_badger_random_delivery_silent' panicked at 'no more messages in queue', libcore/option.rs:989:5 note: Run with `RUST_BACKTRACE=1` for a backtrace

@afck is this a testsuite bug or a bug in my code?

afck · 2018-09-16T10:48:22Z

src/subset.rs

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum SubsetOutput<N> {
+    Contribution(N, Vec<u8>),
+    Done(BTreeMap<N, Vec<u8>>),


Is there a reason to include the values here again? The user has already received the values so it would suffice to just return the set of accepted node IDs (N), or even nothing at all.

Since the values can be large, it would be nice to avoid any unnecessary cloning.

I agree. I think we need the set of accepted node IDs (so that we can determine whether we are done), but we don’t need to return it.

src/subset.rs

afck · 2018-09-16T11:53:45Z

tests/subset.rs

-        assert_eq!(inputs[&id], value);
-    }
+    match output {
+        SubsetOutput::Contribution(_, _) => unreachable!(),


This test probably needs to be updated: The algorithm doesn't make the guarantee that all nodes receive the Contributions in the same order. What we need to verify is that:

Each node outputs the same Contributions, although possibly in a different oder. (No duplicates allowed.)

Each node outputs Done after all the Contributions, and no further outputs after Done.

The values in the Contributions are the ones matching the corresponding input (already checked below).

afck · 2018-09-16T12:20:31Z

src/subset.rs

+        &mut self,
+        proposer_id: &N,
+        result: Option<Vec<u8>>,
+    ) -> Option<SubsetOutput<N>> {


Maybe the "true" count check in this method should be moved further down, at least if we now want to output Contributions (but not Done) even before we have num_correct() instances that returned true.

In any case, we need to make sure that for each accepted value (where the agreement instance returns true), we output Contribution exactly once. Note that this can be triggered by two things:

An agreement instance outputs true and we already have the value—the output of the broadcast instance—for it.

A broadcast instance outputs a value and we already have received the output true from the corresponding agreement instance. (Not very likely, but it can happen if our node is lagging.)

This fixes the test suite, while still outputting results early.

There is a testsuite failure in the `dynamic_honey_badger` tests. Is this a testsuite bug?

src/subset.rs

tests/subset.rs

src/subset.rs

src/honey_badger/epoch_state.rs

src/subset.rs

tests/subset.rs

DemiMarie · 2018-09-17T00:37:29Z

@afck Test suite still fails. Is there a bug in https://github.com/DemiMarie/hbbft/blob/1b37e14d865e2443857379c7b5d32f237af75a73/tests/network/mod.rs#L91-L116?

DemiMarie · 2018-09-17T01:04:54Z

@afck what should I change to fix the test suite?

c0gent · 2018-09-17T02:35:13Z

@DemiMarie The most recent error is simply a formatting error. Just run cargo +beta fmt and it should clear up.

afck

Yes, we check both rustfmt and Clippy (currently disabled, I think) in CI.

src/honey_badger/epoch_state.rs

src/subset.rs

tests/subset.rs

DemiMarie · 2018-09-18T00:49:45Z

Is it normal that the testsuite seems to run in an infinite loop?

Edit: Obviously not ― can someone help debug the root cause?

afck

At a glance I couldn't spot the reason for the failing tests so far, but I'm pretty sure something is still wrong with the map field, since it seems to be read but never written.

If the tests are hanging that probably means that the algorithm currently never terminates but keeps sending messages.

Is it the Honey Badger or the Subset test (or both?) that's hanging?

You can run a single test with the environment variable RUST_LOG=hbbft=debug to see all the ~~spam~~ very helpful logs. Often it's easier to just use hbbft=info and add specific info! logs in the code for debugging to find out what exactly is going on.

src/subset.rs

src/honey_badger/epoch_state.rs

Otherwise, we reject all nodes as faulty.

DemiMarie · 2018-09-18T14:38:21Z

All tests pass on my system.

DemiMarie · 2018-09-18T14:39:15Z

@afck Thank you for your suggestion; the logs allowed me to figure out the problem quickly

There is no need to log a quadratic amount of data.

DemiMarie · 2018-09-18T15:43:34Z

Should I squash?

vkomenda · 2018-09-18T15:46:06Z

We can squash-merge in github.

vkomenda

Great work! My congratulations on making this work. I have a few minor remarks. Given those are taken into account, I'm for merging this into master. I don't think the change changes censorship resilience. Maybe @afck and @c0gent have different opinion? In any case, the new feature works and it looks good.

tests/subset.rs

src/honey_badger/epoch_state.rs

the rest of the nodes. Also `panic!` if `Done` is ever not the last value in a series of `SubsetOutput`s.

afck

Looks good! Just a few more minor nit-picks.

The tests time out sometimes (we're working on it); I restarted them.

src/honey_badger/epoch_state.rs

src/subset.rs

tests/subset.rs

DemiMarie · 2018-09-19T19:59:46Z

@vkomenda @afck is this version good?

tests/subset.rs

DemiMarie · 2018-09-19T21:15:06Z

I collected into a BTreeSet, not a Vec. So the order doesn't matter.

…

On Wed, Sep 19, 2018, 4:48 PM Vladimir Komendantskiy < ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In tests/subset.rs <#233 (comment)>: > // Verify that all instances output the same set. - let mut expected = None; + let observer: BTreeSet<_> = network.observer.outputs().iter().cloned().collect(); .iter().cloned().collect() is equivalent to .to_vec(), I think. Also you removed observer.sort(). BTW the test appears to pass without sorting. Maybe for the future (i.e., moving to the new proptest framework) it would still be better to sort. ------------------------------ In tests/subset.rs <#233 (comment)>: > - // The Subset algorithm guarantees that more than two thirds of the proposed elements - // are in the set. - assert!(output.len() * 3 > inputs.len() * 2); - // Verify that the set's elements match the proposed values. - for (id, value) in output { - assert_eq!(inputs[&id], value); + assert_eq!(outputs.len(), actual.len() + 1); + + // The Subset algorithm guarantees that more than two thirds of the proposed elements + // are in the set. + assert!(actual.len() * 3 > inputs.len() * 2); + for (id, value) in actual { + assert_eq!(&inputs[id], value); + } + + assert_eq!(outputs.iter().cloned().collect::<BTreeSet<_>>(), observer); Same observation: .iter().cloned().collect() is equivalent to .to_vec() and you removed outputs.sort(). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#233 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGWB50OVcbMS0yAX-VjLu0EhfL2iKOqks5ucq2wgaJpZM4Wp2j9> .

vkomenda · 2018-09-20T08:03:44Z

A BTreeSet would collapse multiple occurrences of the same output to one while a sorted Vec would keep those occurrences. So a sorted Vec represents a multiset - a set with multiplicities of its elements - rather than a simple set, and is more expressive.

afck

Looks good to me! From my side, this can be merged. 👍
(I added one more minor suggestion but that's just a matter of taste.)

afck · 2018-09-20T09:09:13Z

src/subset.rs

@@ -220,7 +227,22 @@ impl<N: NodeIdT + Rand> Subset<N> {
                return Ok(step);
            }
        };
-        self.broadcast_results.insert(proposer_id.clone(), value);
+
+        let val_to_insert = if let Some(true) = self.ba_results.get(proposer_id) {


No need to pattern-match here: This could just be if Some(true) == self.ba_results.get(proposer_id).

On the other hand, maybe it should be a match instead: I'm wondering whether in the Some(false) case, we should also just insert None, since we won't need the value anyway—we already know it has been rejected?

vkomenda reviewed Sep 14, 2018

View reviewed changes

DemiMarie force-pushed the output-as-received branch from d442643 to ed73c31 Compare September 14, 2018 21:14

DemiMarie commented Sep 15, 2018

View reviewed changes

src/honey_badger/epoch_state.rs Outdated Show resolved Hide resolved

DemiMarie commented Sep 15, 2018

View reviewed changes

src/honey_badger/epoch_state.rs Outdated Show resolved Hide resolved

src/subset.rs Outdated Show resolved Hide resolved

Outputing subset messages as received

dba489a

This outputs subset messages as they are received. All tests pass.

DemiMarie force-pushed the output-as-received branch from 9ff45d4 to dba489a Compare September 15, 2018 16:43

afck requested changes Sep 16, 2018

View reviewed changes

DemiMarie added 2 commits September 16, 2018 12:05

Fix test suite, while still outputing results early

ea07bc7

This fixes the test suite, while still outputting results early.

Actually do the optimization

ec57c14

There is a testsuite failure in the `dynamic_honey_badger` tests. Is this a testsuite bug?

afck requested changes Sep 16, 2018

View reviewed changes

DemiMarie added 2 commits September 16, 2018 20:23

Respond to code review

bc2597e

Document the meaning of None in Subset::broadcast_results

1b37e14

afck requested changes Sep 17, 2018

View reviewed changes

vkomenda reviewed Sep 17, 2018

View reviewed changes

tests/subset.rs Outdated Show resolved Hide resolved

DemiMarie added 4 commits September 17, 2018 10:37

Fix adding Contributions and fault check

fdf64ae

Fix clippy

ea4be1a

Merge branch 'master' into output-as-received

3f8e823

Merge branch 'master' into output-as-received

8c27895

afck requested changes Sep 18, 2018

View reviewed changes

src/subset.rs Outdated Show resolved Hide resolved

src/subset.rs Outdated Show resolved Hide resolved

src/subset.rs Outdated Show resolved Hide resolved

src/honey_badger/epoch_state.rs Outdated Show resolved Hide resolved

DemiMarie force-pushed the output-as-received branch from 2895c55 to 5ea06d0 Compare September 18, 2018 14:12

Keep track of nodes that have sent us valid messages

fce3cf7

Otherwise, we reject all nodes as faulty.

DemiMarie force-pushed the output-as-received branch from 5ea06d0 to fce3cf7 Compare September 18, 2018 14:15

Remove excessive debug logging

5d6a19b

There is no need to log a quadratic amount of data.

vkomenda suggested changes Sep 18, 2018

View reviewed changes

tests/subset.rs Show resolved Hide resolved

src/honey_badger/epoch_state.rs Outdated Show resolved Hide resolved

src/honey_badger/epoch_state.rs Outdated Show resolved Hide resolved

Re-add check that the observer’s values match

dc7b9f2

the rest of the nodes. Also `panic!` if `Done` is ever not the last value in a series of `SubsetOutput`s.

afck reviewed Sep 19, 2018

View reviewed changes

src/honey_badger/epoch_state.rs Outdated Show resolved Hide resolved

src/honey_badger/epoch_state.rs Outdated Show resolved Hide resolved

src/subset.rs Outdated Show resolved Hide resolved

tests/subset.rs Outdated Show resolved Hide resolved

DemiMarie added 2 commits September 19, 2018 15:03

Respond to review

0638135

Rename field

74e44de

vkomenda suggested changes Sep 19, 2018

View reviewed changes

tests/subset.rs Show resolved Hide resolved

tests/subset.rs Show resolved Hide resolved

afck approved these changes Sep 20, 2018

View reviewed changes

vkomenda approved these changes Sep 20, 2018

View reviewed changes

Merge branch 'master' into output-as-received

35bf0a3

afck merged commit 679f578 into poanetwork:master Sep 20, 2018

afck mentioned this pull request Oct 1, 2018

Make Subset optimization optional. #243

Closed

vkomenda mentioned this pull request Oct 24, 2018

Remove synchrony in HB Epoch State #282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Outputing subset messages as received (initial work) #233

Outputing subset messages as received (initial work) #233

DemiMarie commented Sep 14, 2018

vkomenda Sep 14, 2018

DemiMarie Sep 14, 2018

DemiMarie commented Sep 14, 2018

DemiMarie left a comment

DemiMarie commented Sep 15, 2018

afck left a comment •

edited

Loading

afck Sep 16, 2018

DemiMarie Sep 16, 2018

DemiMarie Sep 16, 2018

afck Sep 16, 2018

DemiMarie Sep 16, 2018

afck Sep 16, 2018

afck Sep 16, 2018

DemiMarie commented Sep 17, 2018 •

edited

Loading

DemiMarie commented Sep 17, 2018

c0gent commented Sep 17, 2018

afck left a comment

DemiMarie commented Sep 18, 2018 •

edited

Loading

afck left a comment •

edited

Loading

DemiMarie commented Sep 18, 2018

DemiMarie commented Sep 18, 2018

DemiMarie commented Sep 18, 2018

vkomenda commented Sep 18, 2018

vkomenda left a comment

afck left a comment

DemiMarie commented Sep 19, 2018

DemiMarie commented Sep 19, 2018 via email

vkomenda commented Sep 20, 2018

afck left a comment

afck Sep 20, 2018

Outputing subset messages as received (initial work) #233

Outputing subset messages as received (initial work) #233

Conversation

DemiMarie commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie commented Sep 14, 2018

DemiMarie left a comment

Choose a reason for hiding this comment

DemiMarie commented Sep 15, 2018

afck left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie commented Sep 17, 2018 • edited Loading

DemiMarie commented Sep 17, 2018

c0gent commented Sep 17, 2018

afck left a comment

Choose a reason for hiding this comment

DemiMarie commented Sep 18, 2018 • edited Loading

afck left a comment • edited Loading

Choose a reason for hiding this comment

DemiMarie commented Sep 18, 2018

DemiMarie commented Sep 18, 2018

DemiMarie commented Sep 18, 2018

vkomenda commented Sep 18, 2018

vkomenda left a comment

Choose a reason for hiding this comment

afck left a comment

Choose a reason for hiding this comment

DemiMarie commented Sep 19, 2018

DemiMarie commented Sep 19, 2018 via email

vkomenda commented Sep 20, 2018

afck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afck left a comment •

edited

Loading

DemiMarie commented Sep 17, 2018 •

edited

Loading

DemiMarie commented Sep 18, 2018 •

edited

Loading

afck left a comment •

edited

Loading